Prevent checks from downloading whole repos #28
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
david-baylibre
force-pushed
the
droze/optimize-check
branch
2 times, most recently
from
85e9492 to
06e9945
Compare
|
@david-baylibre please rebase this branch on Then, I will have a look to review this. |
|
The lint errors are still failing. See: https://github.com/makohoek/repo-resource/actions/runs/7788670056/job/21238599510?pr=28 |
david-baylibre
force-pushed
the
droze/optimize-check
branch
from
eaf6737 to
d9902d4
Compare
makohoek
requested changes
Feb 9, 2024
There was a problem hiding this comment.
Hi @david-baylibre ,
Thank you for this PR. It's a really nice improvement.
I had some difficulties to review this in detail because it's a single big commit doing many things.
For the next version, could you please consider splitting this ?
Here is an example of how this could be split:
- requirements: bump up gitrepo version
- common: improve url matching regex for http/https
- common: add url,revision,name,depth when creating a Repo
- common: formatting changes (if you wish to have these)
- Implement getRevision using git ls-remote (and add unit tests for them)
- Migrate CHECK to use git-ls-remote
Does that make sense ?
david-baylibre
force-pushed
the
droze/optimize-check
branch
from
d9902d4 to
c6000bb
Compare
bump gitrepo from 2.31.1 to 2.32.2 Signed-off-by: David Rozé <[email protected]>
improve url matching regex with re module Signed-off-by: David Rozé <[email protected]>
adding repo settings as class attributes makes it easy to use them at any time in class methods, and not only in sync method Signed-off-by: David Rozé <[email protected]>
fix line too long, and remove extra empty lines and unused module repo Signed-off-by: David Rozé <[email protected]>
Setting revision to None isn't the same as not defining the value. It makes the test more reliable Signed-off-by: David Rozé <[email protected]>
david-baylibre
force-pushed
the
droze/optimize-check
branch
2 times, most recently
from
3ea8815 to
e6fe98f
Compare
makohoek
requested changes
Feb 20, 2024
There was a problem hiding this comment.
Hi David,
Thank you for this new version.
I'm seeing some regression on URLS where we use ssh instead of https (with make tests)
https://gist.github.com/makohoek/c13a71ea7513403c42d64b4ac983fc3d
I would like to see those solved before merging.
Anyways, this is way better than the previous one.
I'm not 100% satisfied with the current design. My main complaints are
- the
Versionclass now has 2 representation of the version (a normalized one and the repo version string.) - I also would have like to see some update in the README documentation.
Right now, we don't descibe the new behaviour is not documented in the README:
https://github.com/makohoek/repo-resource?tab=readme-ov-file#check-check-for-new-versions-in-each-project-under-a-manifest
Both are no deal-breakers so I'm willing to merge this.
We can create github issues to follow up on those.
Repo versions are computed from manifests with updated revisions for each project. The repo tool can only generate a manifest with accurate sha1 hashes once all projects have been downloaded locally (repo manifest -r -o new-manifest.xml). This uses a lot of network bandwith, memory, cpu and disk space and is a huge waste of time. git ls-remote is used instead to fetch revisions much quicker before being injected into the original manifest. A new variable check_jobs is used to spawn concurrent processes to make it close to X times faster Adjust jobs and check_jobs variables based on the git servers capabilities/limitations Signed-off-by: David Rozé <[email protected]>
Repo versions are computed from manifests with updated revisions for each project. The repo tool can only generate a manifest with accurate sha1 hashes once all projects have been downloaded locally (repo manifest -r -o new-manifest.xml). This uses a lot of network bandwith, memory, cpu and disk space and is a huge waste of time. git ls-remote is used instead to fetch revisions much quicker before being injected into the original manifest. A new variable check_jobs is used to spawn concurrent processes to make it close to X times faster Adjust jobs and check_jobs variables based on the git servers capabilities/limitations Signed-off-by: David Rozé <[email protected]>
david-baylibre
force-pushed
the
droze/optimize-check
branch
from
e6fe98f to
dfacb8b
Compare
This was referenced Feb 22, 2024
|
Created: to follow up. |
makohoek
approved these changes
Feb 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Repo versions are computed from manifests with updated revisions for each project. The repo tool can only generate a manifest with accurate sha1 hashes once all projects have been downloaded locally (repo manifest -r -o new-manifest.xml). This uses a lot of network bandwith, memory, cpu and disk space and is a huge waste of time.
git ls-remote is used instead to fetch revisions much quicker before being injected into the original manifest. A new variable check_jobs is used to spawn concurrent processes to make it close to X times faster
Adjust jobs and check_jobs variables based on the git servers capabilities/limitations